-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add friendly_id gem and update node paths on changing title #600
Conversation
This solves the issue of updating node titles when titles are changed. Since wiki pages, notes, questions, and maps come under the same umbrella model of drupal_node with varying paths so I have continued using node paths as it is for nodes. Just adding a |
Now coming to the role of friendly_id here. Once node paths are updated older path becomes obsolete. So people will land up with 404's if trying to visit the nodes will old urls. Here comes friendly_id's |
end | ||
# def slug | ||
# self.path.split('/').last | ||
# end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to comment this due to new slug
column introduced in the node table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are slug and path different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the present context path is the full url of a node like /notes/used/07-06-2016/some-test-url
while slug is only the parameterized version of title like some-test-url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just delete this method, sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah now we can remove this.
@jywarren Please review these changes once you are back. I have added tests and also updated previous tests so that these new changes stand right. I have also left comments in the code that will make it easier to follow. Thanks! |
I haven't updated the sqlite and schema files for any of these changes. I will do it with a follow up PR once you merge in both my PR's |
@@ -142,6 +138,7 @@ def update | |||
i.vid = @revision.vid | |||
i.save | |||
end | |||
@node.title = @revision.title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solves #7
end | ||
|
||
def down | ||
remove_index :node, :slug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just two spaces, please, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jywarren Ah overlooked it. Updated.
Ah, so we'd originally decided that we should not ever change the slug once its saved, as that will break any previous links, and for wiki pages, it would move the wiki page itself. So I think we must ask around if this new behavior is OK (on the dev list, maybe, and also @ebarry)? Just to be clear, it would:
What happens for wiki pages? What if you move a wiki page and then want to make a new wiki page where the old one was? What if you want to change it back to the original title but the redirect record exists? If you retitle it twice -- does it doubly redirect? In general, I like the simpler |
43b7584
to
c625c3f
Compare
@jywarren I have made some changes to the code. Please review. Now the answers to your questions. Friendly_id has its own system to handle duplicate slugs. So all slugs are unique and no two slugs are the same. So if there is already a slug named some
So using friendly_id ensures that no wiki pages are lost and also the slug generated is unique and older urls are redirected back to the newer urls. |
@jywarren I will add some more tests to cover the cases you mentioned. |
Well we could use just friendly_id slugs for node paths as friendly_id itself handles paths but I didn't want to completely switch over to friendly_id as it would involve changing a lot of code and the current path column had to be discarded. So to keep the best of both worlds so that node paths update as well as old urls are not lost I used the current path column along with friendly_id. To use the friendly_id |
Ah, but just to be clear, sorry -- if you have a page like Second, do I understand correctly then, that Thanks, this is a bit subtle and we need to have a clear understanding -- and to ensure people can get the URL they need on a wiki page. Note that for research notes the URL must simply be permanent for linking to (and exactly what the URL becomes is not that important-- it can have "-2" at the end with no problem), but for wikis, having a "good" or recognizable short URL is important too, and "losing" the ability to put a page at a well-known URL is more serious. |
If you have a page like /wiki/barnraising which you then change to Yes its somewhat like that. The node.path is the primary path and the slug On Fri, Jul 8, 2016 at 10:13 AM, Jeffrey Warren notifications@github.com
|
Ah, hmmm. So you wouldn't be able to use /wiki/barnraising anymore? I think
|
I am trying to figure out how to reuse the old urls. But do we really need On Fri, Jul 8, 2016 at 11:00 PM, Ananyo Maiti ananyoevo@gmail.com wrote:
|
Currently friendly_id doesn't support this but I saw a PR On Fri, Jul 8, 2016 at 11:13 PM, Ananyo Maiti ananyoevo@gmail.com wrote:
|
Hi, @ebarry and @steviepubliclab - would like your input here on if and why it's important to be able to preserver old URLs in the (not common) case that:
Ananyo's done some great work here, but as it stands, it would make step 2 above impossible -- the If that's OK, then I can merge this, but it's not the current behavior, and I thought we'd like to retain more flexibility over URLs. |
I think both Liz and Stevie are off today. I'll try to get their input on Monday; thanks for your patience! We just had the Regional Barnraising event in California this past weekend, so folks are still catching up (and catching their breath!) |
No problem. Waiting for the reviews. @jywarren In the mean time I will add tests for the cases you pointed out. I am quite late in my schedule. Trying to manage it. |
Hello! Thanks for working on this and pinging me in. I have a couple thoughts on the URL/Redirect idea. I like the idea that the titles/urls would match if a title is changed, also like that there would be a redirect from previous URLs to the new page, but I'm worried about a situation where someone would want to make a new wiki with an old name. If there was a way to override the redirect in creating a new page, I think it's a great move. |
@steviepubliclab As I have already discussed if we want to make a new wiki with a old name an url with an unique identifier will be used such as |
Is there any way to delete a redirect record? Or is that what the norman On Mon, Jul 18, 2016 at 11:54 AM, Ananya Maiti notifications@github.com
|
02a8bff
to
c968e35
Compare
@jywarren Following your suggestion I found a solution for it. We can reuse the old slugs for new wiki pages if we delete the old slugs on new wiki creation. This way it works as expected. Now new wiki pages can use the old urls of another wiki. I have added some tests to justify it. Please have a look. |
Oh i'm so excited! I've been eager to merge this. Just to confirm, the old On Thu, Jul 21, 2016 at 10:13 PM, Ananya Maiti notifications@github.com
|
Ok I have thought of something so that we won't have to change the format of the urls. I will be making an update to this PR soon. |
Thanks, Ananyo -- I'm happy to take a close look and offer input. I On Aug 1, 2016 6:26 PM, "Ananya Maiti" notifications@github.com wrote:
|
@@ -90,8 +90,10 @@ def raw | |||
|
|||
def show | |||
if params[:author] && params[:date] | |||
@node = DrupalNode.where(path: "/notes/#{params[:author]}/#{params[:date]}/#{params[:id]}").first | |||
finder = "#{params[:author]} #{params[:date]} #{params[:id]}".parameterize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jywarren This keeps the url format intact. I am just creating a finder string from the parameters that finds the appropriate node using the friendly_id slug. Thus we can use best of both worlds! Not modifying the url format as well as using the friendly_id for old url redirects. while updating the url as the title changes.
@jywarren I have solved the problem discussed earlier. Also rebased on the latest master. Please review the changes. |
Wow, exciting; checking this out now. |
slug.blank? || title_changed? | ||
end | ||
|
||
def pretty_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be called "friendly_id_generator" or "friendly_id_adaptor" as the string it returns is not actually used as a URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or friendly_id_string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
friendly_id_string
sounds perfect
This looks great. I just added a couple small requests and we're basically done. Thanks! |
@jywarren I have updated the PR as you requested. Also updated the schema file.Please review. |
Create migrations for slug column and friendly_id_slug table
Add update method to update node path Modify show action in controllers for finding object by slugs
Create friendly_id_slugs fixture for history module
Add integration tests for corresponding changes
…rmat intact Define down method for rename long column migration
11db795
to
2ddd336
Compare
Running tests... this is a huge update, Ananyo! |
We're going to have to watch out extra carefully for any issues that crop up, but your tests look quite thorough. Bundle updated, migrations run... tests running... All good! Wow. Merging... |
Congratulations, this was a big one. Thanks for sticking with it and for all your hard work on it! |
@jywarren Thanks for patience and time Jeff. You also stuck to it till the end. Once again to remind you you need to run this command in the rails console to initialize all the slug column of the nodes. DrupalNode.find_each(&:save) I will also look at any issues that may creep up after this and solve it asap. Thanks a lot Jeff! |
Oh, I should've mentioned, you can add that command to the migration-- On Aug 12, 2016 3:15 PM, "Ananya Maiti" notifications@github.com wrote:
|
Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!
rake test
schema.rb.example
has been updated if any database migrations were addedImportant
To make use of the changes we need to populate the new
slug
column for the existing data in the database. So we have to run this query from the rails console.Thanks!